Code Review

This pull request updates the budget aggregator in sweep/aggregator.go to account for an extra budget contributed by an auxiliary sweeper when filtering inputs. Feedback suggests passing the underlying input.Input interface (pi.Input) instead of the wrapper pi (*SweeperInput) to ExtraBudgetForInputs to avoid type assertion failures in external implementations like Taproot Assets.

This PR fixes a real accounting bug: filterInputs previously gated each input on its own budget alone, ignoring the extra budget an aux sweeper contributes to the set the input joins, which could permanently strand small custom-channel-output sweeps after fee-rate ratcheting. The core fix — folding extraBudget into the minFee and startingFee comparisons — is sound and well-motivated.

Two concerns warrant attention before merge. First, the new aux-budget lookup's error path does continue, which silently drops the input from the round — a new exclusion mode that did not exist when these guards were purely economic field reads, and one that recreates the "silently stranded" failure shape the PR sets out to eliminate. Second, the extra budget is fetched per input over a singleton ([]input.Input{pi.Input}) and added to each input's own budget, whereas the budget input set accounts for it once per set; whether per-input crediting of a set-level pool is correct depends on ExtraBudgetForInputs semantics I cannot see in the diff.

Findings: 🔴 0 Blocker · 🟠 2 Major · 🟡 2 Minor · 🔵 0 Nit

The two concerns that asked for concrete code changes are resolved: the aux-error path now falls back to extraBudget = 0 instead of dropping the input (prior F1), and TestBudgetAggregatorFilterInputsAuxBudget now exercises the rescue, error-keeps, and error-drops cases (prior F4).

The singleton-vs-set accounting question (prior F2) is only partially resolved. The added comment documents that per-input crediting is correct "so long as the aux contribution is additive," but the code still relies on that property of an out-of-tree AuxSweeper without enforcing it or making it verifiable from the diff — three independent review passes re-raised the same divergence. The per-round-cost observation (prior F3) is unchanged; the author declined to batch, with a stated rationale. Two new minor concerns: the externally-sourced extra budget is added without a non-negative/overflow clamp, and one test stub is set conditionally in a way that turns a regression into a mock panic rather than a clean assertion.

Findings: 🔴 0 Blocker · 🟠 1 Major · 🟡 3 Minor · 🔵 0 Nit

Status of prior findings

• F1 addressed: Fixed in sweep/aggregator.go:268 — the aux-lookup error branch now logs and sets extraBudget = 0, then falls through to budget := pi.params.Budget + extraBudget, so a transient aux failure gates the input on its own budget rather than evicting it via continue. This is the conservative behavior the finding requested.
• F4 addressed: Covered by TestBudgetAggregatorFilterInputsAuxBudget in sweep/aggregator_test.go, which exercises (a) aux budget rescuing a low-own-budget input, (b) an aux error keeping an own-budget-sufficient input via the zero fallback, and (c) an aux error still dropping a below-min-fee input. This locks in both the fix and the fallback path.

This revision cleanly addresses four of the six prior findings: the aux error path now falls back to a zero extra budget instead of dropping the input (F1), the negative-clamp and saturating add are in place (F5), the conditional test stub is now unconditional (F6), and the previously-absent regression tests landed (F4). The non-aux path is provably unchanged (fn.MapOptionZ over None yields zero), so inputs without a resolution blob are unaffected.

Two concerns remain, both minor. The singleton-vs-set consistency (F2) is now defined by the new AuxSweeper contract, but that contract is enforced by docstring alone — a pre-existing out-of-tree implementation that predates it can silently violate additivity and desync the per-input filter credit from the per-set construction total. The per-input aux call inside the every-round loop (F3) is still O(n) per round with no batching, timeout, or caching. One genuinely-new minor: the freshly-added clamp/saturation branches have no direct test.

Nothing here rises to blocker or major on the current head.

Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit

Status of prior findings

• F1 addressed: Fixed. The ExtraBudgetForInputs error branch now logs and sets extraBudget = 0, falling through to gate on the input's own budget rather than continue-dropping it — the stranding-on-error mode is gone.
• F4 addressed: Fixed. TestBudgetAggregatorFilterInputsAuxBudget now exercises the aux-rescue, aux-error-with-sufficient-own-budget, and aux-error-below-min-fee branches with a mocked AuxSweeper.
• F5 addressed: Fixed. extraBudget is clamped to >= 0 and folded into the own budget with a saturating add (overflow clamped to math.MaxInt64), so the guard now only ever relaxes the filter relative to the input's own budget.
• F6 addressed: Fixed. The RequiredTxOut stub is now registered unconditionally with .Maybe(), so a regression that let the drop case fall through to the dust check surfaces as a clean assertion failure rather than an unstubbed-mock panic.

This revision resolves the remaining open items. The aux-error path now falls back to a zero contribution instead of dropping the input (F1), the negative-clamp and saturating-add guards are in place (F5), the RequiredTxOut stub is registered unconditionally (F6), and the test now pins both defensive branches with dedicated negative-clamp and overflow-saturation cases (F7). No prior finding remains unresolved.

Two minor observations remain on the newly-added defensive block, both grounded in the current diff and neither blocking: an asymmetry in the overflow guard (it clamps extraBudget but not the input's own budget), and per-input error logging that can amplify a single aux fault. The core fix — folding the aux contribution into the per-input filter so small custom-channel outputs aren't permanently stranded — is sound and well-tested.

Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit

Status of prior findings

• F1 addressed: Fixed in sweep/aggregator.go:268 — the aux-error branch now logs and sets extraBudget = 0, falling through to gate on the input's own budget rather than continue-ing the input out of the round.
• F4 addressed: Fixed in sweep/aggregator_test.go — TestBudgetAggregatorFilterInputsAuxBudget now covers the aux-rescue and aux-error paths with a mocked AuxSweeper, locking in the extraBudget term against future refactors.
• F5 addressed: Fixed in sweep/aggregator.go:285 — extraBudget is clamped to >= 0 and added with a saturating add (extraBudget > math.MaxInt64-budget → MaxInt64), so an out-of-tree aux returning a negative or overflowing value can no longer tighten the filter or wrap negative.
• F6 addressed: Fixed in sweep/aggregator_test.go — mockInput.On("RequiredTxOut").Return(nil).Maybe() is now registered unconditionally, so a regression letting the drop case reach the dust check surfaces as a clean assertion failure rather than an unstubbed-mock panic.
• F7 addressed: Fixed in sweep/aggregator_test.go — the test table now includes "negative aux clamps to zero" (fn.Ok(-shortfall), expect kept) and "overflow saturates instead of wrapping" (near-MaxInt64 own budget plus large positive extra, expect kept), pinning the two defensive branches added for F5.